Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure tab state is torn down on window close #3364

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

graeme
Copy link
Collaborator

@graeme graeme commented Oct 7, 2024

Task/Issue URL: https://app.asana.com/0/1201048563534612/1208463834186044/f

Description:

For some weeks we've had reports about music playing after a browser window playing music is closed. Turns out an indirect reference to the WebView was being held through the ActiveDomainPublisher introduced during #3076. I've made an attempt at fixing this, but lemme know if there's a better way.

Steps to test this PR:

  1. Open a single browser window.
  2. Go to soundcloud.com and start any music playing
  3. Close the window
    4. Music should stop playing

Definition of Done:


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@graeme graeme requested a review from diegoreymendez October 7, 2024 13:46
@diegoreymendez
Copy link
Contributor

diegoreymendez commented Oct 8, 2024

Hey @graeme, I'm a bit confused by this change. I'm adding a bit of context around this issue to explain why.

  1. You're right that the referenced PR introduced a bug.
  2. As far as I was aware that bug should've been fixed here by making the reference weak, meaning it can no longer retain a tab.
  3. It seems to me this change is clearing what's already a weak reference.

Are you sure this is resolving the reported issue? I could not reproduce / validate the original issue through the proposed testing steps.

Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've managed to reproduce the issue. The trick (which I missed) is to not have any other browser window open and just open a single window to play the audio on.

I could repro the issue in main but not here, so I'm approving.

I do think this should be targetting at least the internal branch, though. Please consider retargetting.

PS: it's still unclear to me why this is fixing a retain cycle, but I'll look at that separately.

@graeme graeme changed the base branch from main to release/1.109.0 October 8, 2024 13:33
@graeme graeme merged commit 78caeb3 into release/1.109.0 Oct 8, 2024
26 checks passed
@graeme graeme deleted the graeme/fix-audio-playing-on-closing-window branch October 8, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants